Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix types/findDup() #4303

Merged
merged 1 commit into from
May 8, 2019
Merged

Fix types/findDup() #4303

merged 1 commit into from
May 8, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented May 8, 2019

The function does not work as expected - denoms need
to be compared, not coins.

Add unit test.

Thanks: @zhuliting for pointing this out

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio alessio marked this pull request as ready for review May 8, 2019 11:47
@alessio alessio requested review from fedekunze and sabau May 8, 2019 11:51
@alessio
Copy link
Contributor Author

alessio commented May 8, 2019

Issue was pointed out in #4302

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

Merging #4303 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4303      +/-   ##
==========================================
+ Coverage   59.12%   59.12%   +<.01%     
==========================================
  Files         217      217              
  Lines       14605    14606       +1     
==========================================
+ Hits         8635     8636       +1     
  Misses       5332     5332              
  Partials      638      638

alexanderbez
alexanderbez suggested changes May 8, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a pending log entry stated NewCoins is fixed by way of fixing findDup.

@alessio
Copy link
Contributor Author

alessio commented May 8, 2019

@alexanderbez done, thanks

@alessio
Fix types/findDup()
79a0e35 
The function does not work as expected - denoms need
to be compared, not coins.

Add unit test.

Thanks: @zhuliting for pointing this out
@alessio alessio force-pushed the alessio/fix-types-find-dup branch from 0b21f4d to 79a0e35 Compare May 8, 2019 12:43
@alexanderbez alexanderbez merged commit 04ed12d into master May 8, 2019
@alexanderbez alexanderbez deleted the alessio/fix-types-find-dup branch May 8, 2019 13:51
@alessio alessio mentioned this pull request May 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez approved these changes

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@fedekunze fedekunze Awaiting requested review from fedekunze

@sabau sabau Awaiting requested review from sabau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alessio @alexanderbez